Bluetooth: Separate ISO from AUDIO and HCI HOST only #31182
Conversation
github-actions
bot
added
area: API
|
Shouldn't there instead be some build-time condition for the controller-side ISO support? |
As we briefly discussed in Slack, this file is really neither host nor controller. If anything, we should have some sort of common ISO condition. The scenario where I needed to remove this, was with a non-zephyr controller, but where I was using the rpmsg, so a Zephyr controller ISO config wouldn't really have an effect here either. I'm very open to other ideas/solutions. |
|
Kconfig symbols that are used in both are defined outside of both the controller and host folder, at the moment audio is inside of host, which is why we end up not having this definable for a controller-only build. |
|
One of the other CONFIGs used in this file is One solution is to move the line that sources the Audio Kconfig so that it doesn't require BT_HCI_HOST to be enabled (which is in fact the real problem here) Edit: BT_HCI_HOST and BT_HCI_RAW are btw mutually exclusive, so we can't just enable BT_HCI_HOST either |
|
@Thalley hci_raw.c is never built with the host (as you noticed they're mutually exclusive). It's always built with an HCI driver however, which may be a native controller. I'm pretty sure we have shared Kconfig variables for other things, so seems like these should be moved there. |
|
Just to follow up on my previous comment: enabling BT_HCI_HOST is not the right solution since hci_raw.c is a replacement for the host and should never be built together with it. Instead whatever Kconfig symbols are needed in hci_raw.c should be moved to some place that's independent of the host. Would it be less confusing to move hci_raw.c out from |
|
I also have a feeling that we have some confusion of terms here. When I talk about "the host" I'm talking about the full host stack in |
I think we might have been mixing the terms indeed - I've been referring to the host as either the BT_HCI_HOST or the BT_HCI_RAW :) That being said, I think it makes sense to move any such configs to a common place (we already have a common Kconfig file for that). CONFIG_BT_ISO is a bit... Difficult to deal with here, as that has been defined as a hidden option, which is enabled by enabling the I'd like to get some input from @Vudentz here, as he designed it (the ISO configs) in the first place. |
We could possible move CONFIG_BT_ISO as that is an HCI feature, BT_AUDIO_UNICAST is more of a interface with the host stack itself so I don't think we will be needing it on the likes of hci_raw.c or does the controller actually reuse the same definitions of buffer from the host? At least for ACL it doesn't seems to be the case as the likes of BT_ACL_RX_COUNT are defined in host/Kconfig, perhaps just the BT_MAX_ISO_CONN might be useful for the controller. |
@Vudentz The hci_raw.c/h files use Kconfigs from both the host and the controller for ACL, e.g. I agree that we should not enable |
|
Related to that is also: #30552 |
Thalley
changed the title
subsys/bluetooth/Kconfig
Outdated
There was a problem hiding this comment.
We might want to create something like BT_CIS then since MAX_CONN should only apply to CIS, that said the buffer definition might be useful for BIS as well.
There was a problem hiding this comment.
See https://github.com/zephyrproject-rtos/zephyr/pull/28409/files#r521374652
We can rename it to streams. What do you think?
There was a problem hiding this comment.
Renamed to ISO_CHAN
subsys/bluetooth/Kconfig
Outdated
There was a problem hiding this comment.
What "Isochronous TX buffers" is this referring to? HCI ISO Data packets? SDUs? PDUs? The terminology may have been clear on the host side, but it is in my opinion too fuzzy when being shared with the controller.
(also: Numer->Number)
There was a problem hiding this comment.
I fixed the numer->number part. For the other part, I agree that it is a bit fuzzy, and probably not something I can answer fully. My understanding of hci_raw.c is that it's partially host and partially controller. As far as I can tell, it's being used in the host for ISO Data packets (either over HCI or proprietary), but also for HCI in setups using raw_hci or rpmsg.
There was a problem hiding this comment.
I guess it quite obvious it is for ISO data packets as SDU, etc, are in the controller side but perhaps this will get more confusing if we put this as common Kconfig.
Thalley
force-pushed
the
hci_raw_iso
branch
3 times, most recently
from
d1f7ba2 to
b6ac308
Compare
Thalley
changed the title
|
I updated the PR based on some build errors that was found if e.g. BT_AUDIO was not enabled. |
subsys/bluetooth/Kconfig
Outdated
There was a problem hiding this comment.
I think we will need to revisit this once there is more "meat" on the controller. For the controller it is more natural to model CISes/CIGs/BISes/BIGs than individual channels. But it looks reasonable enough for now.
There was a problem hiding this comment.
I agree. For the host, it doesn't matter if the ISO channel is for a BIS or a CIS.
There was a problem hiding this comment.
Btw., in a discussion with @asmk-ot it came up that this is actually useful as it is for the ISOAL in the controller
subsys/bluetooth/Kconfig
Outdated
There was a problem hiding this comment.
I think this should refer to "HCI ISO Data TX buffers". The controller also has to deal with SDUs and PDUs, so the description should be explicit about this to avoid confusion what the setting refers to.
There was a problem hiding this comment.
As mentioned in another comment: This isn't just for HCI, but is also used in the host if ISO isn't done over HCI.
As I understand it, it's used for full SDUs. If fragmentation is used/supported, the buffer count is controller by CONFIG_BT_ISO_TX_FRAG_COUNT.
I updated the help message to clarify SDUs.
subsys/bluetooth/Kconfig
Outdated
There was a problem hiding this comment.
I assume this is referring to fragmenting SDUs in the host and not to fragmentation in ISOAL?
subsys/bluetooth/Kconfig
Outdated
There was a problem hiding this comment.
Does this refer to the MTU of HCI Isochronous Data packets over HCI? How does it relate to ISO_Data_Packet_Length return parameter of the HCI_LE_Read_Buffer_Size command?
There was a problem hiding this comment.
It is somewhat related to the length returned by the HCI_LE_Read_Buffer_Size command:
max_num = MIN(rp->iso_max_pkt, CONFIG_BT_ISO_TX_BUF_COUNT);
k_sem_init(&bt_dev.le.iso_pkts, max_num, max_num);
The bt_dev.le.iso_pkts is initialized the smallest of the two. How that affects the host is outside of my understanding of how this is implemented. Perhaps @Vudentz can further explain this.
subsys/bluetooth/Kconfig
Outdated
There was a problem hiding this comment.
Why is this limited to 2000 and not to, e.g., 4095, the maximum SDU length?
There was a problem hiding this comment.
It should be 4095. I'll update that.
subsys/bluetooth/Kconfig
Outdated
There was a problem hiding this comment.
Why is this limited to 2000 and not to, e.g., 4095, the maximum SDU length?
subsys/bluetooth/Kconfig
Outdated
There was a problem hiding this comment.
I think this should refer to "HCI ISO Data RX buffers". The controller also has to deal with SDUs and PDUs, so the description should be explicit about this to avoid confusion what the setting refers to.
Also, Numer -> Number
There was a problem hiding this comment.
Updated to specify SDUs
|
@wopu-ot You have a lot of good points w.r.t the ISO Kconfigs (though these were just moved in the PR). It does make me wonder if it makes more sense to move some of them back into the HCI_HOST Kconfigs instead? What we really need in the PR is actually just the HCI Buf count and the CONFIG_BT_ISO option. At this point (with ISO support in the controller in such an early state) I think it's difficult to define such Kconfigs that works for both the host and controller. What do you think? |
|
I think sharing the configuration as you do in this change is actually a good idea, as it minimizes possible mismatches between host and controller. I am just picky about the terminology because it needs to be more specific if there are more possible interpretations. |
Agreed. I'm not sure what the resulting terminology should be. E.g. the |
This was model after existing ACL Kconfig options, we can obviously change that if we want but until there is an actual ISO-AL it is hard to tell how useful these are going to be for the controller. |
|
Ping @asbjornsabo as you probably need to be in the loop of this PR to do rebase after merging. |
|
@nashif Can we merge this, or do you prefer waiting to after the release? |
This commit moves the BT_ISO to a common (host and controller) Kconfig and fixes the ISO buffers in hci_raw.c Signed-off-by: Emil Gydesen <[email protected]>
ISO is a building block for BT_AUDIO but it is not only useful for AUDIO, and as such should be possible to enable without enabling BT_AUDIO. This commit moves iso.c and iso_internal.h to the host directory (from host/audio) and removes the CMakeLists.txt. The /audio directory is left intact for the Kconfig options it provides, and as a directory for future BLE Audio content. Signed-off-by: Emil Gydesen <[email protected]>
A line of code was guarded by CONFIG_BT_L2CAP_TX_FRAG_COUNT instead of CONFIG_BT_ISO_TX_FRAG_COUNT. Signed-off-by: Emil Gydesen <[email protected]>
6 participants
This PR has been updated to not only move the ISO Kconfig options, but the entire ISO implementation. The reasons for this is that BT_ISO Kconfigs should be available for both HCI Host and HCI Raw builds, and that ISO should also be available for other than BT_AUDIO builds.